-
Notifications
You must be signed in to change notification settings - Fork 123
staticaddr: minor fixes #956
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
5b287c8
to
9f7726a
Compare
staticaddr/deposit/manager.go
Outdated
} | ||
wg.Wait() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this commit makes sense. If we recover into the action waitforexpirysweptaction
this recover will block until the tx is confirmed, making loop unusable for uncertain block times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A solution would be to make the action non-blocking (e.g. by moving receiving from the notifiers into a goroutine and sending an event to the fsm)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the catch @sputn1ck. I removed this commit.
9f7726a
to
22266d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Not sure if the second commit is needed.
if err != nil { | ||
log.Errorf("Error sending OnStart event: %v", | ||
err) | ||
} | ||
|
||
m.activeLoopIns[swapHash] = fsm | ||
}() | ||
}(fsm, loopIn.SwapHash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? fsm
and swapHash
are a local variable and they can't change outside of the goroutine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
loopIn
is a iteration variable which might change before the goroutine launches, so this is needed.
22266d6
to
80808b6
Compare
80808b6
to
d526192
Compare
depends on #954, ignore first commit